-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add prototype of mac open command #131213
Conversation
👍 , Some high level feedback:
|
I added in I got an idea from your comment, though, and tried
As for the open command, it provides |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some more feedback in code. I still think we need to find an alternative solution for exec-path
, i.e. we cannot really ship that CLI argument imho.
@bpasero it turns out that one can use the following command for the open call instead of having to add an Earlier attempts weren't working for me because
|
I did some more testing with the |
Ok, it seems the case above comes from me adding a wait arg for both VS Code and open for the |
I have switched the implementation to use a file watcher, and made a new file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments inline.
Besides, on Windows/Linux we are specifically dealing with waitMarkerFilePath
and stdinFilePath
but I am not seeing relevant code for the macOS case now. Specifically:
vscode/src/vs/code/node/cli.ts
Lines 330 to 331 in 77905c8
// Complete when wait marker file is deleted | |
whenDeleted(waitMarkerFilePath!).then(resolve, resolve); |
and
vscode/src/vs/code/node/cli.ts
Lines 334 to 337 in 77905c8
// Make sure to delete the tmp stdin file if we have any | |
if (stdinFilePath) { | |
unlinkSync(stdinFilePath); | |
} |
3ca8543
to
9fd7736
Compare
Even when One use case where both
|
For a long time we used to call
|
It should be fine to use the api since we now launch the app in a way expected by the OS. But the problem of double icon should definitely not happen when using the singleinstance api as all the work will be done by the first instance including waiting on the marker file. I am not sure why this is happening with today's code path, IIUC of the wait logic,
Step 5) should have made the second icon from the dock dismissed, atleast in singleinstance api case Step 5) will be moved up to Step 3) so we definitely don't have to handle hiding the dock icon. |
The cli process should wait on the marker file not on the application process, |
Open's wait flag is enough for --verbose, --status, and - arguments.
Co-authored-by: Benjamin Pasero <benjpas@microsoft.com>
Co-authored-by: Raymond Zhao <raymondzhao@microsoft.com>
e10aa38
to
7c7016e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on the open
command usages
This PR fixes #60579 and fixes #102975.
It switches the spawn call on macOS to use the
open
command, so that the application opens as though it were launched from the dock. In turn, launching the application from the console should now not result in as many bugs such as duplicated recently opened icons.To test on macOS:
code-cli.sh
in the terminal, e.g../scripts/code-cli.sh <your-flags-here>
.